Reviewed 14 of 14 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @YairVaknin-starkware)

vm/src/vm/runners/builtin_runner/mod.rs line 202 at r2 (raw file):

                    Some(0) => Ok(0),
                    Some(ratio) => {
                        if !vm.disable_trace_padding {

please explain (doc) why it isn't relevant with disable_trace_padding

vm/src/vm/runners/builtin_runner/mod.rs line 919 at r2 (raw file):

        // Ensure the last step is not a power of two - true for this specific program, not always.
        assert!(!last_step_true.is_power_of_two());

if you know the exact number, maybe best to assert it's that number. (but do comment that the important thing here is to assert it did not round to a power of 2).
Same above - you can check the exact power of 2 you expect.

vm/src/vm/runners/builtin_runner/mod.rs line 929 at r2 (raw file):

        for (builtin_runner_false, builtin_runner_true) in builtin_runners_false
            .iter()
            .zip(builtin_runners_true.iter())

consider using zip_eq
This isn't checking everything if they are not of the same length.

vm/src/vm/runners/builtin_runner/mod.rs line 222 at r1 (raw file):

                        let allocated_instances = if vm.disable_trace_padding {
                            div_ceil(numerator, ratio as usize)

Please add doc saying why it's ok that it's not accurate (not used, right?) and why it's related to no padding.

vm/src/vm/runners/builtin_runner/mod.rs line 228 at r1 (raw file):

                        };

                        Ok(allocated_instances)

suggestion

Suggestion:

                        let ratio_den = self.ratio_den().or(1);
                        let numerator = vm.current_step * ratio_den as usize;

                        let allocated_instances = if vm.disable_trace_padding {
                            div_ceil(numerator, ratio as usize)
                        } else {
                            safe_div_usize(numerator, ratio as usize)
                                .map_err(|_| MemoryError::ErrorCalculatingMemoryUnits)?
                        };

                        Ok(allocated_instances)

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @yuvalsw)

vm/src/vm/runners/builtin_runner/mod.rs line 222 at r1 (raw file):

Done

vm/src/vm/runners/builtin_runner/mod.rs line 228 at r1 (raw file):

Done

vm/src/vm/runners/builtin_runner/mod.rs line 202 at r2 (raw file):

Done

vm/src/vm/runners/builtin_runner/mod.rs line 919 at r2 (raw file):

I think I prefer it the way it is now. How strongly do you feel about changing it?

vm/src/vm/runners/builtin_runner/mod.rs line 929 at r2 (raw file):

They don't import the needed crate in cargo.toml, and would like to avoid adding to it.
So just added an assertion they are of equal lengths.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @YairVaknin-starkware)

vm/src/vm/runners/builtin_runner/mod.rs line 222 at r1 (raw file):

also "saying why it's ok that it's not accurate" please

vm/src/vm/runners/builtin_runner/mod.rs line 919 at r2 (raw file):

Not super strong. Just generally - tests should have less code and be more specific so that you can be more certain their code itself is good (so they test what you want).

vm/src/vm/runners/builtin_runner/mod.rs line 205 at r3 (raw file):

                            // if we disable trace padding, we don't need to check the min_step,
                            // since we won't have the guarantee of number of steps being padded to
                            // accommodate each builtin size.

please move to above the if (as it refers to what doesn't happen in the if...).

Code quote:

                            // if we disable trace padding, we don't need to check the min_step,
                            // since we won't have the guarantee of number of steps being padded to
                            // accommodate each builtin size.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @yuvalsw)

vm/src/vm/runners/builtin_runner/mod.rs line 222 at r1 (raw file):

Done.

vm/src/vm/runners/builtin_runner/mod.rs line 919 at r2 (raw file):

I agree. And here this is the thing we want to test.

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @YairVaknin-starkware)

a discussion (no related file):
Blocking for now as we need to understand if the other logic we talked about overrides this one or lives along it. If it overrides, then maybe no need to merge this one. You can change this PR to the new logic or send a different one, as you see fit.

Reviewable status: 13 of 14 files reviewed, 3 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @pefontana, and @yuvalsw)

a discussion (no related file):

Changed to a different logic as discussed

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @pefontana, and @YairVaknin-starkware)

vm/src/vm/runners/builtin_runner/mod.rs line 223 at r6 (raw file):

                                .map_err(|_| MemoryError::ErrorCalculatingMemoryUnits)?
                        };

please revert this newline (just to avoid having traces here while you didn't actually change anything here).

vm/src/vm/runners/builtin_runner/mod.rs line 538 at r6 (raw file):

                let used_cells = self.get_used_cells(&vm.segments)?;
                if vm.disable_trace_padding {
                    // If trace padding is disabled, we pad the used cells to still ensure that the

Sorry for nagging, but can you please doc the effects of disable_trace_padding in its definition? I'd like to avoid the case of proof_mode in which no one knows what exactly it does and thus are worried changing/touching it.

vm/src/vm/runners/builtin_runner/mod.rs line 545 at r6 (raw file):

                    } else {
                        0
                    };

assert that padded_used_cells >= used_cells?
Or maybe even compute the used_cells as used_instances * cells_per_instance, and assert it is equal to used_cells?
The computation seems messy and I am concerned it may have inconsistencies.
Or maybe just compute it that way, and convert the assert with a test (asserting both ways of computation yield the same numbers, for all the builtins).

vm/src/vm/runners/builtin_runner/mod.rs line 935 at r6 (raw file):

        for (builtin_runner_false, builtin_runner_true) in builtin_runners_false
            .iter()
            .zip(builtin_runners_true.iter())

this assumes they are in the same order. Makes sense, but consider documenting it.

Code quote:

        for (builtin_runner_false, builtin_runner_true) in builtin_runners_false
            .iter()
            .zip(builtin_runners_true.iter())

vm/src/vm/runners/builtin_runner/mod.rs line 945 at r6 (raw file):

            let (_, allocated_size_false) = builtin_runner_false
                .get_used_cells_and_allocated_size(&runner_false.vm)
                .unwrap_or((0, 0));

any reason this should be Err? If not, just unwrap should do better.

Code quote:

unwrap_or((0, 0));

vm/src/vm/runners/builtin_runner/mod.rs line 954 at r6 (raw file):

            )
            .unwrap();
            let allocated_instances_true = safe_div_usize(

Suggestion:

            let n_allocated_instances_false = safe_div_usize(
                allocated_size_false,
                builtin_runner_false.cells_per_instance() as usize,
            )
            .unwrap();
            let n_allocated_instances_true = safe_div_usize(

vm/src/vm/runners/builtin_runner/mod.rs line 966 at r6 (raw file):

                allocated_instances_true == 0
                    || allocated_instances_true != allocated_instances_false
            );

unclear. What does it check?

Code quote:

            // True for this specific program, not always. That is, instance could could happen to
            // be the same.
            assert!(
                allocated_instances_true == 0
                    || allocated_instances_true != allocated_instances_false
            );

vm/src/vm/runners/builtin_runner/mod.rs line 223 at r5 (raw file):

                        let numerator = vm.current_step * ratio_den as usize;

                        let allocated_instances = if vm.disable_trace_padding {

How come this is not needed? Aren't the safe_divs failing? Ratio is still irrelevant in the case of disable_trace_padding.

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @pefontana, and @yuvalsw)

vm/src/vm/runners/builtin_runner/mod.rs line 223 at r5 (raw file):

We don't reach here when disabling trace padding. TAL at fn get_used_cells_and_allocated_size.

vm/src/vm/runners/builtin_runner/mod.rs line 223 at r6 (raw file):

Done.

vm/src/vm/runners/builtin_runner/mod.rs line 538 at r6 (raw file):

it's defined in the config of the runner, then passed to the vm. In which struct would you like it documented (or both)? Also, not sure I get you remark regarding proof_mode.

vm/src/vm/runners/builtin_runner/mod.rs line 545 at r6 (raw file):

the >= check is done implicitly. get_used_instances just does div_ceil(used_cells, cells_per_instance()). Also, the second remark isn't always true for the last instance (some outputs might not be filled).

vm/src/vm/runners/builtin_runner/mod.rs line 935 at r6 (raw file):

Yeah, it could be assumed, but you're right that it's much clearer if state explicitly. I added an assert at the start of the loop that that's the case.

vm/src/vm/runners/builtin_runner/mod.rs line 945 at r6 (raw file):

Done.

vm/src/vm/runners/builtin_runner/mod.rs line 966 at r6 (raw file):

That there was indeed a different outcome in the number of allocated instances (basically, that true didn't pad according to the ratio)

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @pefontana, and @YairVaknin-starkware)

vm/src/vm/runners/builtin_runner/mod.rs line 538 at r6 (raw file):

Both. I'd write it in the outer one (runner), and in the vm just refer to it for more details.

re proof_mode - it has many effects, it got to the state where it's hard to tell exactly what they are. I don't want disable_trace_padding to get to the same state.

vm/src/vm/runners/builtin_runner/mod.rs line 967 at r7 (raw file):

            // Checks that the number of allocated instances is diffrent when trace padding is
            // disabled. True for this specific program, not always. That is, instance could could
            // happen to be the same.

clearer than before. My suggestion for more clarity:

Suggestion:

            // Checks that the number of allocated instances is diffrent when trace padding is
            // enabled/disabled. Holds for this specific program, not always (that is, in other programs, padding may be of size 0).

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @pefontana, and @yuvalsw)

vm/src/vm/runners/builtin_runner/mod.rs line 538 at r6 (raw file):

Done.

vm/src/vm/runners/builtin_runner/mod.rs line 967 at r7 (raw file):

Done.

Reviewable status: 10 of 14 files reviewed, 5 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @yuvalsw)

Reviewed 3 of 3 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @YairVaknin-starkware)

vm/src/cairo_run.rs line 38 at r9 (raw file):

    pub secure_run: Option<bool>,
    // disable padding of the trace to accommodate the expected ratios/n_steps relationships w.r.t
    // the layout.

Please mention how. It doesn't pad steps at all, but does pad segments to a power of 2...

Suggestion:

    // Disable padding of the trace to accommodate the expected ratios/n_steps relationships w.r.t
    // the layout.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @yuvalsw)

vm/src/vm/runners/builtin_runner/mod.rs line 545 at r6 (raw file):

Done.

Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @YairVaknin-starkware)

vm/src/cairo_run.rs line 39 at r10 (raw file):

    // Disable padding of the trace to accommodate the expected ratios-n_steps relationships w.r.t
    // the layout. It does, however, pad the number of builtin instances to the next power of 2
    // compared to their values at the end of the execution, but doesn't modify n_steps.

It confuses with double negations. My suggestion:

Suggestion:

    /// Disable padding of the trace.
    /// By default, the trace is padded to accommodate the expected builtins-n_steps relationships according to
    /// the layout.
    /// When the padding is disabled:
    /// - It doesn't modify/pad n_steps.
    /// - It still pads each builtin segment to the next power of 2 (counting instances of the builtin) compared to their sizes at the end of the execution.

vm/src/vm/runners/builtin_runner/mod.rs line 543 at r10 (raw file):

                        let n_output_cells =
                            self.cells_per_instance() as usize - self.n_input_cells() as usize;
                        assert!(

1. Please doc what you explained to me f2f - about why it is not exactly equal.
2. why not assert in a test?
3. can the >= become >? Shouldn't there be at least one full output cell ? (not super important of course)

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @yuvalsw)

vm/src/cairo_run.rs line 39 at r10 (raw file):

Done.

vm/src/vm/runners/builtin_runner/mod.rs line 543 at r10 (raw file):

1. Done.
2. thought it couldn't hurt, but moved it to the test and made it more robust (more programs/more used builtins).
   3 . no, for range_check there's no input/output distinction.

Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @YairVaknin-starkware)

vm/src/vm/runners/builtin_runner/mod.rs line 985 at r11 (raw file):

                // the number of used_cells might not be a multiple of cells_per_instance, so we
                // make sure that the discrepancy is up to the number of output cells.
                // This is the same for both cases, so we only check one (true).

IMO wouldn't hurt to test both. It should be the same for both, and this is exactly why we should test this hold for both.

Code quote:

so we only check one (true).

Reviewed 2 of 2 files at r12, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @fmoletta, @FrancoGiachetta, @gabrielbosio, @igaray, @juanbono, @OmriEshhar1, @Oppen, @pefontana, and @YairVaknin-starkware)

Looks good, I've left a small comment